Skip to content

Revise type_param delegation #5505

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Revise type_param delegation #5505

wants to merge 3 commits into from
+33 −35

Conversation

@fingolfin
Copy link
Member

@fingolfin fingolfin commented Oct 29, 2025

Roughly use these "rules"

  • For elements (i.e. objects whose type is a subtype of SetElem; plus some others like SMat), use "type plus parent"
    • although we actually really only need the parent here: if we are willing to change the format we could omit it, as the parent and its type uniquely determine the type of these elements!
    • Doing that would actually also be the first step towards the optimization I suggested at Deserialization is too slow (for the wrong reasons) #4558 (comment)
  • For "parents" such as rings, group, default to just the type.
  • For ideals, matrix space, polynomial rings, the documentation makes it clear that the base resp. coefficient ring is the correct reference object.

I believe these rules are on the long run simpler than what we currently have, and will be easier to document and understand. We should probably also use coefficient_ring for more ring types (resp. implement it first for more) -- it is semantic versus the generally unspecified behaviour of base_ring, and for a serialization format I believe that we should use the semantic option.

So e.g. for SeriesRing on the long run we should ideally use coefficient_ring instead of base_ring, but we currently can't. Luckily @SoongNoonien has already been working on this in Nemocas/AbstractAlgebra.jl#2185

@fingolfin fingolfin added serialization release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Oct 29, 2025
@fingolfin
Copy link
Member Author

fingolfin commented Oct 29, 2025

Actually, I guess we could keep having type_params(R::NCRing) by default via something like

# rings have their base_ring as reference object, if they have one
type_params(x::T) where T <: NCRing = TypeParams(T, base_ring_type(x) !== Union{} && base_ring(x))

Though this leaves some odd cases, e.g. base_ring(QQ) === ZZ but this "base ring" is really just "virtual" in the sense that does not need ZZ to "construct" QQ.

antonydellavecchia
antonydellavecchia reviewed Oct 29, 2025
View reviewed changes
src/Serialization/Fields.jl
# field of rationals (singleton type)
@register_serialization_type QQField
# exclude from ring union definition
type_params(::QQField) = TypeParams(QQField, nothing)
Copy link
Collaborator

@antonydellavecchia antonydellavecchia Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little confused what's happening with QQ now that this has been removed. For example

julia> Oscar.Serialization.type_params(QQ)
Type parameters for QQField ZZ

The printing could be improved... but the point is the return is ZZ and not nothing because of

type_params(R::T) where T <: FracField = TypeParams(T, base_ring(R))

However, QQField still seems to serialize properly including examples when QQField is a parameter.

Copy link
Collaborator

@antonydellavecchia antonydellavecchia Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, there is a catch on line 234 of main.jl

antonydellavecchia
antonydellavecchia approved these changes Oct 29, 2025
View reviewed changes
Copy link
Collaborator

@antonydellavecchia antonydellavecchia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. AbstractAlgebra.Set is useful.

I agree with your remarks about coefficient_ring and we should move away from using base_ring.

Not sure how to deal with type_params for QQField, but it doesn't affect the serialization so this should be good to go.

@antonydellavecchia
Copy link
Collaborator

antonydellavecchia commented Oct 29, 2025

Actually, I guess we could keep having type_params(R::NCRing) by default via something like

# rings have their base_ring as reference object, if they have one
type_params(x::T) where T <: NCRing = TypeParams(T, base_ring_type(x) !== Union{} && base_ring(x))

Though this leaves some odd cases, e.g. base_ring(QQ) === ZZ but this "base ring" is really just "virtual" in the sense that does not need ZZ to "construct" QQ.

this ends up being related to my other comment, but the catch I have in main.jl should also take care of this in the same way.

@fingolfin
Copy link
Member Author

fingolfin commented Nov 5, 2025

Sorry for the late reply: the difference for QQField is because it is a subtype of FracField which I simply did not realize -- and for FracField the delegation is via base_ring. I think the fix is to restrict this to AbstractAlgebra.Generic.FracField

@fingolfin
Copy link
Member Author

fingolfin commented Nov 5, 2025

Ah but since you are handling singletons separately perhaps this is already good enough... the only thing I'd worry about is when this is used recursively. So e.g. if I serialize [QQ, QQ, QQ] then the type params of that list will be derived from the type params of QQ...

(Crazy idea: perhaps the TypeParam constructors should check for this: "if the type is a singletone type, then ignore the params")

@fingolfin
Revise type_param delegation
3017e2e 
For elements, use type plus parent.

For rings, default to just the type.
@fingolfin
typos
280a407
@fingolfin
No params in TypeParams for singleton types
710e899
@antonydellavecchia
Copy link
Collaborator

antonydellavecchia commented Nov 6, 2025

So with new changes we should be able to remove this singleton check on line 234 of main.jl.

Latest commits look good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@antonydellavecchia antonydellavecchia antonydellavecchia approved these changes

Assignees

No one assigned

Labels

release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes serialization

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fingolfin @antonydellavecchia